Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: add /debug/certificates #16087

Merged
merged 1 commit into from
May 24, 2017
Merged

UI: add /debug/certificates #16087

merged 1 commit into from
May 24, 2017

Conversation

mberhault
Copy link
Contributor

@mberhault mberhault commented May 23, 2017

Fixes #15027

Simple html page in front of /debug/certificates.
Just lists some relevant fields from x509.Certificate.

eg: for a simple "single CA cert" and "single local node" page:
certs

I think these are pretty much all the interesting fields, at least for the certs we generate.

@mberhault mberhault requested a review from BramGruneir May 23, 2017 19:09
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mberhault mberhault force-pushed the marc/debug_certificates branch 3 times, most recently from bab162a to b55542e Compare May 24, 2017 01:20
@mberhault
Copy link
Contributor Author

Added list of valid addresses to cockroach cert list command. The other fields aren't too useful to have in that command, and space is limited:
cert_list

@BramGruneir
Copy link
Member

:lgtm:


Reviewed 6 of 6 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


pkg/cli/cert.go, line 218 at r2 (raw file):

		}

		addRow(cert, fmt.Sprintf("user: %s", user))

why have the user variable at all? Just add the row, either unknown or the common name?


pkg/server/debug_certificates.go, line 34 at r1 (raw file):

)

// Returns an HTML page displaying information about all node's view of a

This comment is incorrect.


pkg/server/debug_certificates.go, line 73 at r1 (raw file):

type perCertificateWebData struct {
	CertificateType string
	Certs           []*x509.Certificate

Certs is not displayed in the template, doesn't need to be capitalized. Might not even be required in the struct.


pkg/server/debug_certificates.go, line 91 at r1 (raw file):

	}

	for i, r := range response.Certificates {

nit, use c or cert instead of r here


pkg/server/debug_certificates.go, line 217 at r1 (raw file):

    <DIV CLASS="wrapper">
      <H1>Node {{.NodeID}} certificates</H1>
      {{range .Certificates}}

I tend to put a {{- for all single template lines so the html looks nicer.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


pkg/cli/cert.go, line 218 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

why have the user variable at all? Just add the row, either unknown or the common name?

Because node certificates display addresses: ... in the same field. We need to specify what it's for.


pkg/server/debug_certificates.go, line 34 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This comment is incorrect.

Done.


pkg/server/debug_certificates.go, line 73 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Certs is not displayed in the template, doesn't need to be capitalized. Might not even be required in the struct.

Done.


pkg/server/debug_certificates.go, line 91 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

nit, use c or cert instead of r here

Done.


pkg/server/debug_certificates.go, line 217 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I tend to put a {{- for all single template lines so the html looks nicer.

Done, though I'm not sure I buy the "nicer". The indentation in the template is based on the block structures as opposed to HTML hierarchy so the resulting html is already weird.


Comments from Reviewable

@mberhault mberhault force-pushed the marc/debug_certificates branch from 310b08e to cbe3465 Compare May 24, 2017 14:38
@BramGruneir
Copy link
Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/cli/cert.go, line 218 at r2 (raw file):

Previously, mberhault (marc) wrote…

Because node certificates display addresses: ... in the same field. We need to specify what it's for.

I think you misunderstood. I mean in this case, why not just

if cert.Error == nil && len(cert.ParsedCertificates) > 0 {
	addRow(cert, fmt.Sprintf("user: %s", cert.ParsedCertificates[0].Subject.CommonName))
 } else {
	addRow(cert, "user: unknown")
}

pkg/server/debug_certificates.go, line 217 at r1 (raw file):

Previously, mberhault (marc) wrote…

Done, though I'm not sure I buy the "nicer". The indentation in the template is based on the block structures as opposed to HTML hierarchy so the resulting html is already weird.

Yeah, the spacing isn't perfect. There's no perfect way to go about this.


Comments from Reviewable

@mberhault
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


pkg/cli/cert.go, line 218 at r2 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I think you misunderstood. I mean in this case, why not just

if cert.Error == nil && len(cert.ParsedCertificates) > 0 {
	addRow(cert, fmt.Sprintf("user: %s", cert.ParsedCertificates[0].Subject.CommonName))
 } else {
	addRow(cert, "user: unknown")
}

bah, same thing. I've been going with "build up the notes then do one call".


Comments from Reviewable

Fixes #15027

Simple html page in front of `/debug/certificates`.
Just lists some relevant fields from `x509.Certificate`.
@mberhault mberhault force-pushed the marc/debug_certificates branch from cbe3465 to a4c15f7 Compare May 24, 2017 15:11
@mberhault mberhault merged commit 5b53b0b into master May 24, 2017
@mberhault mberhault deleted the marc/debug_certificates branch May 24, 2017 15:36
knz added a commit to knz/cockroach that referenced this pull request Jul 6, 2022
Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we
introduced a certificate rotation mechanism. To help
teach/troubleshoot that feature, we also provided a way for the
operator to view the certificate details in the DB Console (expiration
time, addresses, etc.)

This work was done in PR cockroachdb#16087, to solve issues cockroachdb#15027/cockroachdb#1674.

However, as part of that PR, the implementation of the back-end API
also included the *data* of the cert (including the cert signature
and the signature chain) in the response payload.

This additional payload was never used in a user-facing feature: the
DB Console does not display it nor does it contain a link to "download
the cert file". The back-end API is not public either, so we are not
expecting end-users to have legitimate uses for this feature.

Meanwhile, leaking cert data through an API runs dangerously close
to violating PCI guidelines (not quite, since keys are not exposed,
but still...).

So in order to avoid a remark on this during PCI review cycles, and to
remove the chance this will be misused, this patch removes the
data payload from the cert response.

The DB Console screen corresponding to the original work remains
unaffected.

Release note: None
knz added a commit to knz/cockroach that referenced this pull request Jul 6, 2022
Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we
introduced a certificate rotation mechanism. To help
teach/troubleshoot that feature, we also provided a way for the
operator to view the certificate details in the DB Console (expiration
time, addresses, etc.)

This work was done in PR cockroachdb#16087, to solve issues cockroachdb#15027/cockroachdb#1674.

However, as part of that PR, the implementation of the back-end API
also included the *data* of the cert (including the cert signature
and the signature chain) in the response payload.

This additional payload was never used in a user-facing feature: the
DB Console does not display it nor does it contain a link to "download
the cert file". The back-end API is not public either, so we are not
expecting end-users to have legitimate uses for this feature.

Meanwhile, leaking cert data through an API runs dangerously close
to violating PCI guidelines (not quite, since keys are not exposed,
but still...).

So in order to avoid a remark on this during PCI review cycles, and to
remove the chance this will be misused, this patch removes the
data payload from the cert response.

The DB Console screen corresponding to the original work remains
unaffected.

Release note: None
craig bot pushed a commit that referenced this pull request Jul 6, 2022
83616: sql, sqlstats: create temporary stats container for all txns r=xinhaoz a=xinhaoz

# Commit 1
### sql: add txn_fingerprint_id to node_statement_statistics

This commit adds the `txn_fingerprint_id` column to
`crdb_internal.node_statement_statistics`.

Release note (sql): `txn_fingerprin_id` has been added to
`crdb_internal.node_statement_statistics`. The type of the
column is NULL or STRING.

------------------------------
# Commit 2
### sql, sqlstats: create temporary stats container for all txns

Fixes: #81470

Previously, the stats collector followed different procedures for stats
collection depending on whether or not the txn was explicit.

For explicit transactions, all the stmts in the txn must be recorded with
the same `transactionFingerprintID`, which is  only known after all stmts
in the txn have been executed. In order to record the correct
txnFingerprintID, a temporary stats container was created for stmts in the
current transaction. The `transactionFingerprintID` was then populated for
all stmts in the temp container, and the temp container was merged with
the parent.

For implict transactions, the assumption was there would only be a single
stmt in the txn, and so no temporary container was created, with stmts
being written directly to the application stats.

This assumption was incorrect, as it is possible for implicit txns to have
multiple stmts, such as stmts sent in a batch.

This commit ensures that stats are properly collected for implicit txns
with multiple stmts. The stats collector now follows the same procedure
for both explicit and implicit txns, creating a temporary container for
local txn stmts and merging on txn finish.

Release note (bug fix): Statement and transaction stats are now properly
recorded for implicit txns with multiple stmts.

83762: docs: add MVCC range tombstones tech note r=sumeerbhola,jbowens,nicktrav a=erikgrinaker

[Rendered version](https://github.com/erikgrinaker/cockroach/blob/mvcc-range-tombstones-tech-note/docs/tech-notes/mvcc-range-tombstones.md)

---

This describes the current state, but the details will likely change as
we iterate on the implementation.

Resolves #83406.

Release note: None

83873: tenantsettingswatcher: remove the version gate r=yuzefovich a=yuzefovich

This commit removes the version gate for the tenant settings.

Release note: None

83902: server: remove TLS cert data retrieval over HTTP r=catj-cockroach a=knz

Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we
introduced a certificate rotation mechanism. To help
teach/troubleshoot that feature, we also provided a way for the
operator to view the certificate details in the DB Console (expiration
time, addresses, etc.)

This work was done in PR #16087, to solve issues #15027/#1674.

However, as part of that PR, the implementation of the back-end API
also included the *data* of the cert (including the cert signature
and the signature chain) in the response payload.

This additional payload was never used in a user-facing feature: the
DB Console does not display it nor does it contain a link to "download
the cert file". The back-end API is not public either, so we are not
expecting end-users to have legitimate uses for this feature.

Meanwhile, leaking cert data through an API runs dangerously close
to violating PCI guidelines (not quite, since keys are not exposed,
but still...).

So in order to avoid a remark on this during PCI review cycles, and to
remove the chance this will be misused, this patch removes the
data payload from the cert response.

The DB Console screen corresponding to the original work remains
unaffected.

For reference here's how the console screen looks: 
![image](https://user-images.githubusercontent.com/642886/177591040-f554fdf0-2b04-48f6-af36-0b94c0bcaf4c.png)


Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
knz added a commit to knz/cockroach that referenced this pull request Jul 12, 2022
Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we
introduced a certificate rotation mechanism. To help
teach/troubleshoot that feature, we also provided a way for the
operator to view the certificate details in the DB Console (expiration
time, addresses, etc.)

This work was done in PR cockroachdb#16087, to solve issues cockroachdb#15027/cockroachdb#1674.

However, as part of that PR, the implementation of the back-end API
also included the *data* of the cert (including the cert signature
and the signature chain) in the response payload.

This additional payload was never used in a user-facing feature: the
DB Console does not display it nor does it contain a link to "download
the cert file". The back-end API is not public either, so we are not
expecting end-users to have legitimate uses for this feature.

Meanwhile, leaking cert data through an API runs dangerously close
to violating PCI guidelines (not quite, since keys are not exposed,
but still...).

So in order to avoid a remark on this during PCI review cycles, and to
remove the chance this will be misused, this patch removes the
data payload from the cert response.

The DB Console screen corresponding to the original work remains
unaffected.

Release note: None
knz added a commit to knz/cockroach that referenced this pull request Aug 3, 2022
Back in CockroachDB v1.1 (v17.2 in the new calver scheme), we
introduced a certificate rotation mechanism. To help
teach/troubleshoot that feature, we also provided a way for the
operator to view the certificate details in the DB Console (expiration
time, addresses, etc.)

This work was done in PR cockroachdb#16087, to solve issues cockroachdb#15027/cockroachdb#1674.

However, as part of that PR, the implementation of the back-end API
also included the *data* of the cert (including the cert signature
and the signature chain) in the response payload.

This additional payload was never used in a user-facing feature: the
DB Console does not display it nor does it contain a link to "download
the cert file". The back-end API is not public either, so we are not
expecting end-users to have legitimate uses for this feature.

Meanwhile, leaking cert data through an API runs dangerously close
to violating PCI guidelines (not quite, since keys are not exposed,
but still...).

So in order to avoid a remark on this during PCI review cycles, and to
remove the chance this will be misused, this patch removes the
data payload from the cert response.

The DB Console screen corresponding to the original work remains
unaffected.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants